Skip to content

test: increase coverage for URL.searchParams#10952

Closed
hiroppy wants to merge 2 commits intonodejs:masterfrom
hiroppy:feature/add-searchparams-tests
Closed

test: increase coverage for URL.searchParams#10952
hiroppy wants to merge 2 commits intonodejs:masterfrom
hiroppy:feature/add-searchparams-tests

Conversation

@hiroppy
Copy link
Member

@hiroppy hiroppy commented Jan 22, 2017

Add exceptions for all cases.
Add entries, keys and values files.(Validation tests and exception tests are included.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 22, 2017
@hiroppy hiroppy force-pushed the feature/add-searchparams-tests branch 2 times, most recently from 8d0ef6f to c7f159a Compare January 22, 2017 16:22
@mscdex mscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 22, 2017
cjihrig
cjihrig reviewed Jan 22, 2017
View reviewed changes
test/parallel/test-whatwg-url-searchparams-values.js Outdated
Copy link
Contributor

@cjihrig cjihrig Jan 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth checking done here too.

@hiroppy hiroppy force-pushed the feature/add-searchparams-tests branch from c7f159a to 91ec5aa Compare January 22, 2017 21:28
TimothyGu
TimothyGu reviewed Jan 22, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these tests were actually derived from W3C's web-platform-tests, so I'm sure they would appreciate these changes as well.

test/parallel/test-whatwg-url-searchparams-entries.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also useful might be making sure entries[Symbol.iterator]() === entries.

test/parallel/test-whatwg-url-searchparams-entries.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may also test calling entries.next again (to simulate overread).

test/parallel/test-whatwg-url-searchparams-entries.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entries.next.call(undefined) should throw as well.

test/parallel/test-whatwg-url-searchparams-foreach.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems to overlap with a series of similar, but more comprehensive tests in #10905. I'm fine with keeping it in this PR, but I'd prefer just dropping it here.

@TimothyGu
Copy link
Member

TimothyGu commented Jan 22, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/5984/

@hiroppy
Copy link
Member Author

hiroppy commented Jan 22, 2017 •
edited
Loading

@TimothyGu Thank you for reviews. PTAL 9c326fe

EDIT: I'll squash.

@hiroppy hiroppy force-pushed the feature/add-searchparams-tests branch from 9c326fe to b031c07 Compare January 22, 2017 22:46
TimothyGu
TimothyGu suggested changes Jan 28, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abouthiroppy, this PR seems to stop working after ed0086f. Can you please rebase and make sure it works on master? Thanks.

test/parallel/test-whatwg-url-searchparams-entries.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also not needed any more after 326e967.

@hiroppy hiroppy force-pushed the feature/add-searchparams-tests branch from b031c07 to 0626657 Compare January 29, 2017 03:08
@hiroppy
Copy link
Member Author

hiroppy commented Jan 29, 2017

@TimothyGu I rebased and confirmed that the test passed. Thanks.

TimothyGu
TimothyGu reviewed Jan 29, 2017
View reviewed changes
test/parallel/test-whatwg-url-searchparams-keys.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually do you mind fixing this and in -values.js as well?

const URLSearchParams = require('url').URLSearchParams;

Copy link
Member Author

@hiroppy hiroppy Jan 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I forgot to change it.

@hiroppy
test: add tests of entries, keys, values for searchParams
189de74 
Improve coverage for entries, keys and values.
Validation tests and exception tests are included.
@hiroppy hiroppy force-pushed the feature/add-searchparams-tests branch from 0626657 to 189de74 Compare January 29, 2017 08:06
@TimothyGu
Copy link
Member

TimothyGu commented Jan 29, 2017

Updated CI: https://ci.nodejs.org/job/node-test-pull-request/6094/

@TimothyGu
Copy link
Member

TimothyGu commented Jan 29, 2017

Landed in e7f4825.

@TimothyGu TimothyGu closed this Jan 29, 2017
TimothyGu pushed a commit that referenced this pull request Jan 29, 2017
@hiroppy @TimothyGu
test: add tests for searchParams
e7f4825 
PR-URL: #10952
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@hiroppy hiroppy deleted the feature/add-searchparams-tests branch January 29, 2017 09:53
TimothyGu added a commit to TimothyGu/node-review that referenced this pull request Jan 29, 2017
@TimothyGu
review: fix Metadata state operation
3939f18 
E.g. nodejs/node#10952
@TimothyGu TimothyGu mentioned this pull request Jan 29, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@hiroppy @evanlucas
test: add tests for searchParams
fcd08b8 
PR-URL: #10952
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
Merged
evanlucas pushed a commit to nodejs/node-review that referenced this pull request Feb 6, 2017
@TimothyGu @evanlucas
Improvements (#7)
0b323f0 
* index: allow #fragments in PR URLs

Also check tightened PR_RE against pathname

* review: fix Metadata state operation

E.g. nodejs/node#10952

* review: simplify Fixes creation

* review: overhaul getCollaborators()

- Make regex static and more concise
- Iterate over RE.exec
- Use Map

* review: remove extra whitespace

Fixes: #5

* review: only look for LGTMs in <p>'s

Fixes: nodejs/node#10657
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@hiroppy @TimothyGu @jasnell @cjihrig @joyeecheung @mscdex @nodejs-github-bot